Fix the event subscription in dotnet #11
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request modernizes session event handling in the .NET client by migrating from Newtonsoft.Json's JToken to System.Text.Json's JsonElement, improving type safety and consistency across the codebase. It also adds comprehensive test coverage for session event subscriptions.
Changes:
- Replaced
JTokenwithJsonElementfor event deserialization to align with the System.Text.Json serialization stack - Refactored the RPC handler to accept individual parameters instead of a notification object
- Added test coverage for session event subscriptions to verify proper handling of user, assistant, and idle events
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dotnet/src/Client.cs | Updated OnSessionEvent RPC handler to use individual parameters and changed SessionEventNotification to use JsonElement instead of JToken |
| dotnet/test/SessionTests.cs | Added Should_SessionEvt_Subscribed test to verify session event subscription functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SteveSandersonMS
left a comment
SteveSandersonMS
left a comment
There was a problem hiding this comment.
Awesome, thanks @showpune! This is great.
I've triggered a separate run of the E2E tests for this, and it passed. The run associated with this PR failed due to a config issue with secrets, but that's unrelated to the content of this PR, so I'm merging anyway.
2 participants
This pull request updates the way session events are handled and tested in the .NET client, focusing on improving type safety and expanding test coverage for session event subscriptions.
Session Event Handling Improvements:
SessionEventNotification.Eventproperty fromJToken?toJsonElement?for better type safety and consistency in event deserialization.OnSessionEventRPC handler to accept astring sessionIdandJsonElement? @eventinstead of aSessionEventNotificationobject, and adjusted the event parsing logic accordingly.Test Enhancements:
Should_SessionEvt_SubscribedinSessionTests.csto verify that session event subscriptions work as expected, ensuring that user, assistant, and idle events are received during a session.